Hi @benaadams, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

May need changes to Kestrel's custom IDictionaries dealing with empty keys
Also Content-Length bonding to header

@benaadams What you're getting at with much of this is that our multiple levels of abstraction are inefiicent and we should scale back. That's legit, but I think we need to choose which collection we use carefully. We used Dictionaries internally because they were convenient storage, but as fowler pointed out we hate that their index throws, and they also have a significantly larger surface area than we need.

Rather than pushing IDictionary up to the surface APIs, let's try pushing a reduced IHeaderCollection down into the features/servers. we only really need IEnumerable<KeyValuePair<string, StringValues>>, Count, indexer, and ICollection<Keys>. @davidfowl, @lodejard, what do you think?

The same goes for query, form, etc..

@Tratcher we can't push the form and query etc down to the server but that's not what you mean I assume.

@davidfowl Not down to the server, but down to their respective storage/features.

@Tratcher yes. However, after a large amount of changes, and some iterations to maintain a close analogue to flow and constraints, I mostly ended up in a similar place to where the code started; including wrapping on get for headers!

There are a few tweaks but they really end up independent to the abstraction changes (LazyDict and its inherited collections, KeyValueAccumulator). Which was't where I expected to be; and would have likely been the exact same place if I wasn't trying to avoid it. 😿

However, I did ponder along the journey...

Questions

• Is wrapping the collections for no "throw on get" necessary? I completely agree with the reasons; but could this be a documented constraint for writing a custom server or implementing IHttpRequestFeature / IHttpResponseFeature? For example Kestrel's FrameHeaders collection will not throw on get and that will probably be the default for the majority of people. So is this overly defensive?

  Equally the default collections for forms and query are again provided so can be wrapped at that point (as per this PR with FormCollection : SafeLazyDictionary ignoring the inheritance exposure issue)

• Is the request data being read-only desirable? With the request data being read-only this means middleware providers are more likely to resolve down to IHttpRequestFeature and wholesale replace the collection as they can't otherwise alter the data; making the defensive approach in the above question more necessary.

I had a few more, but I think they are artefacts of the above two and what implementation the answers encourage you down. Not necessarily looking for answers.

IDictionary's contract is that the indexer throws, that's not something we can/should change in our stack as it would case a lot of unpleasant surprises. If we want a different semantic then we need a different contract / interface. I don't think we even need IDictionary, it was just the closest match to what wanted. Now that we're cleaning things up I think we can come up with a slimmed down type that has the semantics we want and use that in both the surface API (HttpRequest) and internally (IHttpRequestFeature).

The request data should be mutable. There are common scenarios where middleware will alter the request to meet downstream requirements like applying x-forwarded headers to their respective fields.

Different Contract makes sense; also with IgnoreCase on keys?

If you did something like this (inheritance-wise) everyone will want some:

interface IReadOnly<TKey, TValue>
{
    TValue Item { get; }
}

interface IReadWrite<TKey, TValue> : IReadOnly<TKey, TValue>
{
    new TValue Item { get; set; }
}

It seems to work, though you have to implement the get twice:

class Thing<TKey, TValue> : IReadWrite<TKey, TValue>
{
    public TValue Item
    {
        get
        {
            throw new NotImplementedException();
        }

        set
        {
            throw new NotImplementedException();
        }
    }

    TValue IReadOnly<TKey, TValue>.Item
    {
        get
        {
            return Item;
        }
    }
}

Then you can create a concrete type; use it read/write via interface and expose as readonly without any as casting between IReadWrite and IReadOnly only upcasting.

var thing = Thing();

thing["that"] = that;

IReadWrite iThing = thing;
iThing["other"] = other;

return (IReadOnly)iThing;

Rebased rather than merged

Hi @benaadams, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

@rynowak How badly will this break MVC?

@Tratcher - I'd expect much more breakage in the tests which frequently new up collections for testing. We actually read these very few places in the product code.

This is pretty much done. I'll rebase and merge it when I get a chance later today. Any of the minor remaining comments you don't get to by then I'll just take care of. Thanks

Done, however quick check am returning string.Empty where a cookie doesn't exist which is "" rather than null; is this the expected behavior?

Should be fine. If they really care they can use TryGetValue

Rebased, squashed, merged.

👏 :)